Skip to content

Conversation

@tropnikovvl
Copy link
Contributor

@tropnikovvl tropnikovvl commented Mar 21, 2025

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2025

CLA assistant check
All committers have signed the CLA.

>
<div className={"text-nb-gray-400 font-light truncate"}>
{userOfPeer?.email}
{!userOfPeer?.email && userOfPeer?.id}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could probably be userOfPeer?.email || userOfPeer?.name || userOfPeer?.user_id || userOfPeer?.id:

  • email would come first as usual
  • name second as it's still user readable
  • synchronized user_id from IdP (usually not readable)
  • internal object id for the user (never readable)

the existing construct looks weird to me, but I didn't write JS for a decade so don't take my word on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code and made sure it works.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing that I thought of, we should probably prefix the user_id/id with "user:" string, but I'm not sure how to type it out correctly in TypeScript or JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that it will look like this in the end?
Screenshot 2025-03-21 at 17 28 36

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tropnikovvl exactly what you screenshotted
on the UX side, we could end the series of ||s with "<unknown>" so it's obvious nothing was found instead of id being "undefined" string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, actually we shouldn't output anything at all when the user is undefined, this will simply mean there is no user attached to the peer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like (pseudocode):

user = userOfPeer?.email || userOfPeer?.name
userId = peer?.user_id || userOfPeer?.id
if !user && userId {
	user = `user:${userId}`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done
The user field will only be displayed if there is at least some ID value.

Screenshot 2025-03-21 at 18 26 20

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, @heisbrot can you review and merge this?

@tropnikovvl tropnikovvl requested a review from nazarewk March 21, 2025 13:41
@nazarewk nazarewk requested review from heisbrot and removed request for nazarewk March 21, 2025 15:36
@tropnikovvl
Copy link
Contributor Author

Hi @heisbrot ,
Maybe you have time to look at this PR?

@heisbrot
Copy link
Contributor

Hey @tropnikovvl

right now it shows user: also when there is an email or name.
It would be great if we keep the old behavior and show the user: prefix only if there is no email or name.

@tropnikovvl
Copy link
Contributor Author

Hi @heisbrot ,
Done

@heisbrot heisbrot merged commit 042c65a into netbirdio:main Mar 27, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants